Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revised component catalogue #60

Merged
merged 40 commits into from
Oct 24, 2023
Merged

Revised component catalogue #60

merged 40 commits into from
Oct 24, 2023

Conversation

FDelporte
Copy link
Member

No description provided.

@FDelporte FDelporte requested a review from eitch July 18, 2023 08:28
@FDelporte FDelporte self-assigned this Jul 18, 2023
@sonatype-lift
Copy link

sonatype-lift bot commented Jul 18, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

Copy link
Member Author

@FDelporte FDelporte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too much for a full code review, but after a quick look, I only have small remarks. I love the improvements and additional comments as they make the code cleaner and easier to understand.

pom.xml Outdated
@@ -43,7 +44,7 @@
<pi4j.version>2.3.0-SNAPSHOT</pi4j.version>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be <pi4j.version>2.3.0</pi4j.version>

@@ -1,8 +1,8 @@
package com.pi4j.catalog;

import com.pi4j.context.Context;
import java.time.Duration;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a "beauty remark". When using auto save in IntelliJ IDEA with cleanup, imports should get sorted alphabetically. It's not critical at all, but will avoid code changes between commits, where imports only move place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do a 'optimize imports'

double aIn2 = adc.readValue(Ads1115.Channel.A2);
double aIn3 = adc.readValue(Ads1115.Channel.A3);
System.out.printf("Voltages: a0=%.3f V, a1=%.3f V, a2=%.3f V, a3=%.3f V%n", aIn0, aIn1, aIn2, aIn3);

System.out.println("Single read done.");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid the use of System.out but have all output via logger

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the apps are using System.out and the components use logging.

Thought it might be easier to read.

@eitch What do you think? Use logging everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the logging can output the class name, it should be easy to distinguish the output, but nothing is "set in stone"

new Buzzer.Sound(PAUSE, 8)
);

private final List<Buzzer.Sound> imperialMarch = List.of(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need a nerdy example here? Star Wars? ;-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore above comment :-)

@FDelporte FDelporte merged commit b71a170 into main Oct 24, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants